Fix handling of warnings on DML batches#1643
Conversation
ggilder
requested review from
meiji163,
rashiq and
timvaillancourt
as code owners
There was a problem hiding this comment.
Pull request overview
Improves --panic-on-warnings safety during binlog replay by ensuring warnings emitted by any statement inside a batched DML multi-statement are detected (not just the last statement), addressing the masking/data-loss scenario from #1636 / follow-up to #1633.
Changes:
- Add
executeBatchWithWarningChecking()to interleaveSHOW WARNINGSafter each DML in a batch whenPanicOnWarningsis enabled. - Update
ApplyDMLEventQueries()to use the new warning-checking execution path only forPanicOnWarnings(keep existing fast path otherwise). - Add a new unit test and a new local integration test case to validate “warning in the middle of a batch” behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| localtests/panic-on-warnings-batch-middle/extra_args | Adds localtest arguments enabling --panic-on-warnings with a unique index alter. |
| localtests/panic-on-warnings-batch-middle/expect_failure | Asserts failure output contains warning-detection text. |
| localtests/panic-on-warnings-batch-middle/create.sql | Sets up event-driven DML to reproduce a batched replay scenario. |
| go/logic/applier_test.go | Adds unit test asserting mid-batch warning causes rollback of entire batch. |
| go/logic/applier.go | Implements per-statement warning detection for batched DML execution under PanicOnWarnings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
@meiji163 important correction to earlier data loss fix — lmk if you have questions |
|
Working on fixing test flake issues here — will update when I have a passing build on my fork |
ggilder
force-pushed
the
fix-dml-batch-issue
branch
2 times, most recently
from
2b7f7d9 to
47a5977
Compare
ggilder
force-pushed
the
fix-dml-batch-issue
branch
from
47a5977 to
7245a90
Compare
ggilder
force-pushed
the
fix-dml-batch-issue
branch
from
7245a90 to
1e8f6ce
Compare
|
Rebased on latest master |
Related issue: #1636
Description
This is a follow-up to #1633 — this was actually an incomplete fix for the data loss issue described there, because any DML events that would cause data loss that happened in the middle of a batch could be masked by success of the final statement in the batch. This code change now runs
SHOW WARNINGSafter each statement in a batch so that we collect all warnings.script/cibuildreturns with no formatting errors, build errors or unit test errors.